Add building blocks dropdown #1251
Conversation
| const PatternSelectDropdown = styled(Dropdown)` | ||
| span, | ||
| span.caret { | ||
| color: #333; |
josh-willis-arcadis
requested review from
amy-corson-ibigroup and
daniel-heppner-ibigroup
|
The percy tests caught that there's a black border around the dropdown now. Is that expected? |
|
I noticed this too.... I kinda like it with the metro, and in the route viewer? But it does look a little weird with the old itinerary. |
|
@daniel-heppner-ibigroup @amy-corson-ibigroup I think the border was added by accident tbh. I think it looks great in the route viewer. I will remove the border on the itinerary panel to pass the percy tests. |
There was a problem hiding this comment.
We're repeating these a lot:
line-height: 20px;
padding: 3px 7px;
span.caret {
margin-left: 2px;
}
IF we think this is what the default dropdown should be, let's go ahead and fix it in building-blocks. But I actually think the new dropdown looks nice and is an improvement! Interested in @daniel-heppner-ibigroup opinion on this too, if we prefer to match the percy tests exactly, that's totally fine. But right now we're setting those styles in one place and immediately overriding them in another.
| @@ -149,12 +166,11 @@ class RouteDetails extends Component<Props> { | |||
| <HeadsignSelectLabel htmlFor="headsign-selector-label"> | |||
| <FormattedMessage id="components.RouteDetails.stopsTo" /> | |||
| </HeadsignSelectLabel> | |||
| <SortResultsDropdown | |||
| <PatternSelectDropdown | |||
There was a problem hiding this comment.
If we decide to leave the default margins (which would mean a slightly taller dropdown) just flagging it would probably be good to throw an align-items: center on the parent here so that this row doesn't look uneven:
There was a problem hiding this comment.
This is the only place I really had a problem with the way the new styling looks. If we can get this vertically centered then I'm happy to approve the Percy diff
There was a problem hiding this comment.
Take a look a 78d52eb. This addresses @amy-corson-ibigroup and @daniel-heppner-ibigroup suggestions!
There was a problem hiding this comment.
Looking really good, I would maybe add some margin-bottom to this div just to add a little vertical space between the selector and the "stops in this direction of travel":
There was a problem hiding this comment.
added here! c51ffba
| @@ -149,12 +166,11 @@ class RouteDetails extends Component<Props> { | |||
| <HeadsignSelectLabel htmlFor="headsign-selector-label"> | |||
| <FormattedMessage id="components.RouteDetails.stopsTo" /> | |||
| </HeadsignSelectLabel> | |||
| <SortResultsDropdown | |||
| <PatternSelectDropdown | |||
There was a problem hiding this comment.
Looking really good, I would maybe add some margin-bottom to this div just to add a little vertical space between the selector and the "stops in this direction of travel":
amy-corson-ibigroup
assigned josh-willis-arcadis and unassigned daniel-heppner-ibigroup and amy-corson-ibigroup
|
@daniel-heppner-ibigroup I think the Percy diff is good to be approved. Let me know if there are any more questions! |
|
Looks great! Thank you for your hard work here @josh-willis-arcadis |
daniel-heppner-ibigroup
assigned josh-willis-arcadis and unassigned daniel-heppner-ibigroup
Implements new building block dropdown component.
Addl discussion at #1241
PR Checklist: